C/C++: Detect ambiguous assignment of comparison results - #22336
C/C++: Detect ambiguous assignment of comparison results#22336theinfosecguy wants to merge 2 commits into
Conversation
ryao
left a comment
ryao
left a comment
There was a problem hiding this comment.
While I am happy you did the work for me to get this into a PR and made what appear to be improvements, would you add an Original-patch-by: to the commit message to credit my prior work?
Also, have you run your variant against any major corpora (e.g. Linux, curl, OpenZFS) to verify the lack of FPs in production code, like I did with the original version? If it helps:
| not isExplicitlyGrouped(comparison) and | ||
| occursInCondition(assignment) and | ||
| // Assigning a comparison result to a Boolean is normally intentional. | ||
| not assignment.getLValue().getUnspecifiedType() instanceof BoolType and |
There was a problem hiding this comment.
While this is normally intentional, I believe not isExplicitlyGrouped(comparison) precludes this.
There was a problem hiding this comment.
Why is that? It seems like the types of the expressions and the bracketing are mostly independent concerns.
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
I've reviewed the code and docs from a technical perspective (I haven't looked through all the test cases yet). This looks quite promising. I've also done a mass (MRVA) run and found quite a high rate of true positive results!
Original-patch-by: Richard Yao <richard@ryao.dev>
7784270 to
8f0ea61
Compare
|
QHelp previews: cpp/ql/src/Likely Bugs/Likely Typos/AmbiguousAssignmentOfComparison.qhelpAmbiguous assignment of comparison used as truth valueAssignment operators have lower precedence than comparison operators. For example, RecommendationUse parentheses to make the intended operation order explicit. To assign first and compare the assigned value, parenthesize the assignment. To intentionally assign the comparison result, parenthesize the comparison. An explicit cast around the comparison also makes that order clear. ExampleIn the first condition, int read_status();
int check_status() {
int status;
if (status = read_status() < 0) // BAD: assigns the comparison result.
return status;
if ((status = read_status()) < 0) // GOOD: assigns first, then compares.
return status;
return 0;
}
References
|
geoffw0
left a comment
geoffw0
left a comment
There was a problem hiding this comment.
Nice thorough tests. I've nit-picked a couple of cases but they're not important ones, and you might have more insight into them than I have, from knowing the cases that motivated this query.
I've gained a lot of confidence in this query from reviewing it - just a few small things to discuss and decide if we want to make any changes or not. 👍
geoffw0
added
the
ready-for-doc-review
|
Thank you for making the changes. The CodeQL looks good to me, as do the tests and MRVA (bulk testing) results. I think I said it before, but I'm seeing lots of true positive results in real world data. :) I've requested a quick review of the |
6 participants
Adds
cpp/ambiguous-assignment-of-comparisonto flag ambiguous assignments when the assignment result is used as a truth value, such as:The query distinguishes this from explicitly grouped assign-then-compare and compare-then-assign expressions. It includes C and C++ tests, query help, and query-suite integration.
Local targeted and neighboring tests pass. The motivating regression is detected, and a run against
git/gitproduced no alerts.Fixes #22286